Don't disable other container engines when --vm_driver=none #4545
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @marcosdiez. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-ci-robot
added
the
size/S
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: marcosdiez The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Can one of the admins verify this patch? |
k8s-ci-robot
added
size/M
tstromberg
changed the title
cmd/minikube/cmd/start.go
Outdated
| @@ -672,7 +672,8 @@ func configureRuntimes(runner cruntime.CommandRunner) cruntime.Manager { | |||
| exit.WithError(fmt.Sprintf("Failed runtime for %+v", config), err) | |||
| } | |||
|
|
|||
| err = cr.Enable() | |||
| disableOtherContainerEngines := viper.GetString(vmDriver) != constants.DriverNone | |||
There was a problem hiding this comment.
While this is clever, let's aim for easier readability:
disableOthers := true
if viper.GetString(vmDriver) == constants.DriverNone {
disableOthers = false
}
cr.Enable(disableOthers)
Also, prefer shorter variable names for variables which have a very short scope.
pkg/minikube/cruntime/containerd.go
Outdated
| func (r *Containerd) Enable() error { | ||
| if err := disableOthers(r, r.Runner); err != nil { | ||
| glog.Warningf("disableOthers: %v", err) | ||
| func (r *Containerd) Enable(disableOtherContainerEngines bool) error { |
There was a problem hiding this comment.
Prefer shorter variable names for a short scope, such as disableOthers. Leave the long variable names for globals.
There was a problem hiding this comment.
There was a problem hiding this comment.
Hi, although I agree, I can't do it. We have a function called disableOthers already.
I can't think about another good short name.
Do you have any suggestions ?
marcosdiez
force-pushed
the
dont_disable_container_engines_when_vmdriver_is_none
branch
from
70cdcbd
to
f802c19
Compare
|
Just for the record my build passed although Travis says otherwise: https://travis-ci.org/kubernetes/minikube/builds/549788904?utm_source=github_status&utm_medium=notification |
…ner_engines_when_vmdriver_is_none
|
@minikube-bot OK to test |
pkg/minikube/cruntime/crio.go
Outdated
| func (r *CRIO) Enable() error { | ||
| if err := disableOthers(r, r.Runner); err != nil { | ||
| glog.Warningf("disableOthers: %v", err) | ||
| func (r *CRIO) Enable(disableOtherContainerEngines bool) error { |
There was a problem hiding this comment.
lets change disableOtherContainerEngines to disOthers
|
/retest this please |
|
@minikube-bot OK to test |
closes #4541
it speedups a second on two the minikube launch here !